Fix/SK-1127 | add session_id flag #742
Conversation
KatHellg
requested review from
niklastheman and
Wrede
and removed request for
niklastheman
There was a problem hiding this comment.
Cool, I think I would like to have the "id functionality" (list_clients for example) refactored in separate functions (not part of list). The implementation of this looks good! The "session_id functionality" can be implemented in an other way. The GET [entity]/ endpoints accepts url params for filtering. For example: GET /rounds?session_id=<session_id_value>
| @click.option("-id", "--id", required=False, help="Client ID") | ||
| @client_cmd.command("get") | ||
| @click.pass_context | ||
| def get_client(ctx, protocol: str, host: str, port: str, token: str = None, id: str = None): |
There was a problem hiding this comment.
Think id should be required...
fedn/cli/model_cmd.py
Outdated
|
|
||
| if session_id: | ||
| url = f"{url}?session_id={session_id}" | ||
| headers["session_id"] = session_id |
There was a problem hiding this comment.
This header should not be needed.
fedn/cli/package_cmd.py
Outdated
| @@ -72,13 +103,10 @@ def list_packages(ctx, protocol: str, host: str, port: str, token: str = None, i | |||
| headers["id"] = id | |||
|
|
|||
|
|
|||
| click.echo(f"\nListing packages: {url}\n") | |||
| click.echo(f"\nretrieving package: {url}\n") | |||
There was a problem hiding this comment.
retrieving => Retrieving? :)
fedn/cli/client_cmd.py
Outdated
| @@ -66,18 +97,14 @@ def list_clients(ctx, protocol: str, host: str, port: str, token: str = None, id | |||
| headers["id"] = id | |||
There was a problem hiding this comment.
This header should not be needed.
fedn/cli/client_cmd.py
Outdated
|
|
||
|
|
||
| click.echo(f"\nRetrieving client: {url}\n") | ||
| click.echo(f"Headers: {headers}") |
| @@ -65,12 +65,46 @@ def list_clients(ctx, protocol: str, host: str, port: str, token: str = None, n_ | |||
|
|
|||
| try: | |||
| response = requests.get(url, headers=headers) | |||
| print_response(response, "clients") | |||
| print_response(response, "clients", None) | |||
| except requests.exceptions.ConnectionError: | |||
There was a problem hiding this comment.
We should handle more than just ConnectionError, there are plenty of other error related to http requests. You don't have do do this know but make an other issue of it. For example create a utility function that handles all requests and checks the response for error:
import requests
from requests.exceptions import HTTPError, Timeout, ConnectionError, RequestException
def make_request(method, url, **kwargs):
"""
A reusable function to handle HTTP(S) requests with error handling.
Args:
method (str): HTTP method (e.g., 'GET', 'POST').
url (str): The URL for the request.
**kwargs: Additional arguments passed to `requests.request()`.
Returns:
dict: A dictionary with the response data or an error message.
"""
try:
response = requests.request(method, url, **kwargs)
response.raise_for_status()
# Assuming JSON response, adjust if needed
return {"success": True, "data": response.json()}
except HTTPError as http_err:
return {"success": False, "error": f"HTTP error occurred: {http_err}"}
except Timeout:
return {"success": False, "error": "The request timed out"}
except ConnectionError:
return {"success": False, "error": "Connection error occurred"}
except RequestException as req_err:
return {"success": False, "error": f"Request error: {req_err}"}
except Exception as e:
return {"success": False, "error": f"Unexpected error: {e}"}
fedn/cli/combiner_cmd.py
Outdated
|
|
||
|
|
||
| click.echo(f"\nRetrieving combiner: {url}\n") | ||
| click.echo(f"Headers: {headers}") |
fedn/cli/model_cmd.py
Outdated
| url = f"{url}{id}" | ||
|
|
||
| click.echo(f"\nRetrieving model: {url}\n") | ||
| click.echo(f"Headers: {headers}") |
fedn/cli/shared.py
Outdated
| """Prints the api response to the cli. | ||
| :param response: | ||
| type: array | ||
| description: list of entities | ||
| :param entity_name: | ||
| type: string | ||
| description: name of entity | ||
| :param so: | ||
| type: boolean | ||
| desriptions: single output format (y/n) |
| click.echo(f"\t{k}: {v}") | ||
| click.echo("}") | ||
| else: | ||
| count, result = json_data.values() |
There was a problem hiding this comment.
we should consider using json pretty prints instead, however I would prefer not introducing a new dependency. Save for other issue
added session_id flag in model_cmd.py, round_cmd.py, statuses_cmd.py, and validation_cmd.py. Works well but might be a brute force solution so please lmk if there's a smarter way of doing it.
also added an id flag in each file to be able to search based on id (e.g., model_id in model_cmd.py)